http con man: set response flag on downstream protocol error #8522

spenceral · 2019-10-07T20:18:08Z

This adds a new response flag, DPE, that is set when the downstream
request has an HTTP protocol error.

Signed-off-by: Spencer Lewis slewis@squareup.com

Description: Protocol errors previously resulted in just the DC flag being set. Now, DPE will be set when there is a codec exception caused by the downstream request.
Risk Level: low
Testing: unit test
Docs Changes: update list of response flags
Release Notes: added release note
Fixes #8446

This adds a new response flag, DPE, that is set when the downstream request has an HTTP protocol error. Signed-off-by: Spencer Lewis <slewis@squareup.com>

repokitteh-read-only · 2019-10-07T20:18:12Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #8522 was opened by spenceral.

see: more, trace.

Signed-off-by: Spencer Lewis <slewis@squareup.com>

junr03

a couple comments. Thanks for adding this!

junr03 · 2019-10-10T18:00:06Z

docs/root/intro/version_history.rst

@@ -3,6 +3,7 @@ Version history

 1.12.0 (pending)
 ================
+* access log: added a new flag for downstream protocol error.


do you mind adding a ref to the field?

junr03 · 2019-10-11T16:29:10Z

source/common/http/conn_manager_impl.cc

@@ -348,6 +349,9 @@ void ConnectionManagerImpl::resetAllStreams() {
    auto& stream = *streams_.front();
    stream.response_encoder_->getStream().removeCallbacks(stream);
    stream.onResetStream(StreamResetReason::ConnectionTermination, absl::string_view());
+    if (response_flag) {
+      stream.stream_info_.setResponseFlag(*response_flag);


nit: the style almost everywhere on the codebase is to use has_value() and value()

Ah, that is nicer. Thanks

Signed-off-by: Spencer Lewis <slewis@squareup.com>

spenceral · 2019-10-11T18:50:10Z

Force pushed the last commit because I forgot to sign 🙃

Signed-off-by: Spencer Lewis <slewis@squareup.com>

spenceral · 2019-10-14T14:54:41Z

I think the failing test is a flake. When I locally run

bazel test //test/integration:http_subset_lb_integration_test --nocache_test_results --runs_per_test=100

I get

INFO: Build completed, 1 test FAILED, 101 total actions
//test/integration:http_subset_lb_integration_test                       FAILED in 1 out of 100 in 1.8s

spenceral · 2019-10-17T14:10:34Z

/retest

repokitteh-read-only · 2019-10-17T14:10:38Z

🐴 hold your horses - no failures detected, yet.

🐱

Caused by: a #8522 (comment) was created by @spenceral.

see: more, trace.

spenceral · 2019-10-17T14:11:51Z

@junr03 is there a way to rerun the Azure tests?

junr03 · 2019-10-17T16:35:16Z

/azp run

azure-pipelines · 2019-10-17T16:35:44Z

Azure Pipelines successfully started running 3 pipeline(s).

spenceral · 2019-10-18T15:49:46Z

/azp run

azure-pipelines · 2019-10-18T15:49:50Z

Commenter does not have sufficient privileges for PR 8522 in repo envoyproxy/envoy

Signed-off-by: Spencer Lewis <slewis@squareup.com>

junr03 · 2019-10-21T16:46:22Z

@htuch this change lgtm, do you mind doing the last pass given it needs API approval?

htuch

@envoyproxy/api-shepherds API LGTM. Ready to merge when CI passes.

snowp · 2019-10-22T18:56:34Z

/azp run

azure-pipelines · 2019-10-22T18:57:02Z

Azure Pipelines successfully started running 3 pipeline(s).

mattklein123 · 2019-10-23T04:59:47Z

/azp run

azure-pipelines · 2019-10-23T05:00:05Z

Azure Pipelines successfully started running 3 pipeline(s).

mattklein123 · 2019-10-23T17:12:31Z

/azp run

azure-pipelines · 2019-10-23T17:12:48Z

Azure Pipelines successfully started running 3 pipeline(s).

mattklein123 · 2019-10-23T17:21:59Z

Legit format failure. Please merge master and fix format.

/wait

* master: (54 commits) Update dependencies - Go, Bazel toos, xxHash, nanopb, rules_go, curl, protobuf, Jinja, yaml-cpp, re2 (envoyproxy#8728) test: increase coverage of listener_manager_impl.cc (envoyproxy#8737) test: modify some macros to reduce number of uncovered lines reported (envoyproxy#8725) build: add a script to setup clang (envoyproxy#8682) http: fix ssl_redirect on external (envoyproxy#8664) docs: update fedora build requirements (envoyproxy#8641) fix draining listener removal logs (envoyproxy#8733) dubbo: fix router doc (envoyproxy#8734) server: provide server health status when stats disabled (envoyproxy#8482) router: adding a knob to configure a cap on buffering for shadowing/retries (envoyproxy#8574) tcp proxy: add default 1 hour idle timeout (envoyproxy#8705) thrift: fix filter names in docs (envoyproxy#8726) Quiche changes to avoid gcc flags on Windows (envoyproxy#8514) test: increase test coverage in Router::HeaderParser (envoyproxy#8721) admin: add drain listeners endpoint (envoyproxy#8415) buffer filter: add content-length when ending stream with trailers (envoyproxy#8609) clarify draining option docs (envoyproxy#8712) build: ignore go-control-plane mirror git commit error code (envoyproxy#8703) api: remove API type database from checked in artifacts. (envoyproxy#8716) admin: correct help strings (envoyproxy#8710) ... Signed-off-by: Spencer Lewis <slewis@squareup.com>

ran ./tools/proto_format.sh fix Signed-off-by: Spencer Lewis <slewis@squareup.com>

mattklein123

Thanks!

…oxy#8522) Signed-off-by: Spencer Lewis <slewis@squareup.com>

http con man: set response flag on downstream protocol error

b66ae96

This adds a new response flag, DPE, that is set when the downstream request has an HTTP protocol error. Signed-off-by: Spencer Lewis <slewis@squareup.com>

update grpc access log

1ce35f5

Signed-off-by: Spencer Lewis <slewis@squareup.com>

spenceral force-pushed the downstream-protocol-error branch from 97adccc to 1ce35f5 Compare October 7, 2019 21:05

spenceral added 2 commits October 7, 2019 17:15

update v3 access log proto

c634e50

Signed-off-by: Spencer Lewis <slewis@squareup.com>

update filter accesslog v3 proto

098efdb

Signed-off-by: Spencer Lewis <slewis@squareup.com>

jmarantz requested a review from AndresGuedez October 8, 2019 17:19

jmarantz assigned AndresGuedez and junr03 and unassigned AndresGuedez Oct 8, 2019

jmarantz removed the request for review from AndresGuedez October 10, 2019 12:47

junr03 reviewed Oct 11, 2019

View reviewed changes

pr comments

47f5944

Signed-off-by: Spencer Lewis <slewis@squareup.com>

spenceral force-pushed the downstream-protocol-error branch from 706e274 to 47f5944 Compare October 11, 2019 18:49

try ref again

69c1a0a

Signed-off-by: Spencer Lewis <slewis@squareup.com>

fix merge conflict

4359e13

Signed-off-by: Spencer Lewis <slewis@squareup.com>

repokitteh-read-only bot added the api label Oct 18, 2019

junr03 assigned htuch Oct 21, 2019

htuch previously approved these changes Oct 22, 2019

View reviewed changes

repokitteh-read-only bot removed the api label Oct 22, 2019

mattklein123 self-assigned this Oct 23, 2019

repokitteh-read-only bot added the waiting label Oct 23, 2019

spenceral added 2 commits October 23, 2019 13:52

fix format

e8bbfa2

ran ./tools/proto_format.sh fix Signed-off-by: Spencer Lewis <slewis@squareup.com>

spenceral dismissed htuch’s stale review via e8bbfa2 October 23, 2019 18:02

repokitteh-read-only bot added api and removed waiting labels Oct 23, 2019

mattklein123 approved these changes Oct 23, 2019

View reviewed changes

repokitteh-read-only bot removed the api label Oct 23, 2019

mattklein123 merged commit 6b3df46 into envoyproxy:master Oct 23, 2019

spenceral deleted the downstream-protocol-error branch October 23, 2019 22:07

derekargueta pushed a commit to derekargueta/envoy that referenced this pull request Oct 24, 2019

http con man: set response flag on downstream protocol error (envoypr…

72a1cb1

…oxy#8522) Signed-off-by: Spencer Lewis <slewis@squareup.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http con man: set response flag on downstream protocol error #8522

http con man: set response flag on downstream protocol error #8522

spenceral commented Oct 7, 2019

repokitteh-read-only bot commented Oct 7, 2019

junr03 left a comment

junr03 Oct 10, 2019

junr03 Oct 11, 2019

spenceral Oct 11, 2019

spenceral commented Oct 11, 2019

spenceral commented Oct 14, 2019

spenceral commented Oct 17, 2019

repokitteh-read-only bot commented Oct 17, 2019

spenceral commented Oct 17, 2019

junr03 commented Oct 17, 2019

azure-pipelines bot commented Oct 17, 2019

spenceral commented Oct 18, 2019

azure-pipelines bot commented Oct 18, 2019

junr03 commented Oct 21, 2019

htuch left a comment

snowp commented Oct 22, 2019

azure-pipelines bot commented Oct 22, 2019

mattklein123 commented Oct 23, 2019

azure-pipelines bot commented Oct 23, 2019

mattklein123 commented Oct 23, 2019

azure-pipelines bot commented Oct 23, 2019

mattklein123 commented Oct 23, 2019

mattklein123 left a comment

http con man: set response flag on downstream protocol error #8522

http con man: set response flag on downstream protocol error #8522

Conversation

spenceral commented Oct 7, 2019

repokitteh-read-only bot commented Oct 7, 2019

junr03 left a comment

Choose a reason for hiding this comment

junr03 Oct 10, 2019

Choose a reason for hiding this comment

junr03 Oct 11, 2019

Choose a reason for hiding this comment

spenceral Oct 11, 2019

Choose a reason for hiding this comment

spenceral commented Oct 11, 2019

spenceral commented Oct 14, 2019

spenceral commented Oct 17, 2019

repokitteh-read-only bot commented Oct 17, 2019

spenceral commented Oct 17, 2019

junr03 commented Oct 17, 2019

azure-pipelines bot commented Oct 17, 2019

spenceral commented Oct 18, 2019

azure-pipelines bot commented Oct 18, 2019

junr03 commented Oct 21, 2019

htuch left a comment

Choose a reason for hiding this comment

snowp commented Oct 22, 2019

azure-pipelines bot commented Oct 22, 2019

mattklein123 commented Oct 23, 2019

azure-pipelines bot commented Oct 23, 2019

mattklein123 commented Oct 23, 2019

azure-pipelines bot commented Oct 23, 2019

mattklein123 commented Oct 23, 2019

mattklein123 left a comment

Choose a reason for hiding this comment